Update thumbnails only when they are visible (to improve scrolling through large documents) #5586

fkaelberer · 2014-12-26T18:07:49Z

This PR postpones highlighting the current page thumbnail in PDFThumbnailViewer.scrollThumbnailIntoView() until the thumbnail view is opened. Setting the style of thumbnails can take long and become a bottleneck if the document (and thus, the DOM tree) is very large. Scrolling through very large documents is now smoother with this PR.

Good tests:
[1] pdf (2000+ pages) from #5207 and
[2] pdf (3000+ pages) from https://bugzilla.mozilla.org/show_bug.cgi?id=885287#c6.
[3] pdf (5000+ pages)

A benchmark: Scrolling through the first 50 pages of [3] with the ↓ key:

Browser	w/o patch	w/ patch
FF 34	1:01	0:49
FF Nightly 37.0a1	1:50	1:07

Another benchmark: While scrolling through the first 10 pages of [1] with the ↓ key, the frame rates are now much more often at the 60 fps mark (FF Nightly 37.0a1):

EDIT: Added correct link to [3] which I had used for benchmarking, added Firefox 34 results (which shows performance regression of Nightly)

CodingFabian · 2014-12-26T18:49:26Z

good first step. i think with regards to reflow there are quite a few optimizations possible.

yurydelendik · 2014-12-26T19:25:00Z

really good. is there a reason why we cannot just cancel scrollThumbnailIntoView from 'pagechange' completely when !isThumbnailViewEnabled? (the patch looks a little bit "asymmetrical" IMHO) I'm trying to avoid this.renderingQueue.isThumbnailViewEnabled check and refreshing during its state update from different location (which is still named scrollThumbnailIntoView).

fkaelberer · 2014-12-26T19:56:15Z

Is there a reason why we cannot just cancel scrollThumbnailIntoView from 'pagechange' completely when !isThumbnailViewEnabled?

Maybe not. Returning immediately if !isThumbnailViewEnabled seems to work as well. But I didn't want to be too obtrusive with my changes. Maybe someone (who is more familiar with the viewer) can even find a better patch now that I have pointed to the performance issue.

Also I'm not too happy with the reference to the rendering queue in this.renderingQueue.isThumbnailViewEnabled to check if the sidebar is open. Is there a more direct check?

fkaelberer · 2014-12-27T16:45:36Z

I updated the commit. Now scrollThumbnailIntoView is only called if PDFViewerApplication.sidebarOpen. This is cleaner and addresses @yurydelendik's comments.

CodingFabian · 2014-12-27T19:49:35Z

this solves quite a few concerns for us. thanks!

Snuffleupagus · 2014-12-27T20:31:05Z

I've found one small issue with this patch:
If you manage to open the sidebar when the PDF file is loading, but before the pages have started rendering, only the first two thumbnails are drawn and the following is printed in the console:

TypeError: thumbnail is null                                   thumbnail_view.js:299:6

Probably the easiest way to solve this is to check if (thumbnail) before thumbnail_view.js#L299.

fkaelberer · 2014-12-28T18:27:01Z

@Snuffleupagus: I didn't manage to reproduce the warning, but I added the check anyway. Thanks.

Snuffleupagus · 2014-12-29T10:16:22Z

/botio-linux preview

pdfjsbot · 2014-12-29T10:16:23Z

From: Bot.io (Linux)

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/3c8cc003d4850d0/output.txt

pdfjsbot · 2014-12-29T10:17:10Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/3c8cc003d4850d0/output.txt

Total script time: 0.78 mins

Published

Viewer: http://107.21.233.14:8877/3c8cc003d4850d0/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/3c8cc003d4850d0/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/3c8cc003d4850d0/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/3c8cc003d4850d0/extensions/firefox/pdf.js.amo.xpi

Snuffleupagus · 2014-12-29T10:22:33Z

@Snuffleupagus: I didn't manage to reproduce the warning, but I added the check anyway. Thanks.

I can confirm that this fixes the issue I was seeing!

fkaelberer · 2015-01-02T17:58:19Z

I added a second commit: Update thumbnail images only when sidebar is visible.

This finally makes scrolling smooth as butter, as long as thumbnails are off. Updating the thumbnail images causes one or two reflows (setting the image and setting the data-loaded css class), which were causing the lags.

Browser	w/o patch	w/ patch	w/ 2nd patch
FF 34	1:01	0:49	0:46
FF Nightly 37.0a1	1:50	1:07	0:46

timvandermeij · 2015-01-02T18:40:03Z

/botio-linux preview

pdfjsbot · 2015-01-02T18:40:04Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/16397a37e238e5e/output.txt

pdfjsbot · 2015-01-02T18:40:52Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/16397a37e238e5e/output.txt

Total script time: 0.80 mins

Published

Viewer: http://107.21.233.14:8877/16397a37e238e5e/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/16397a37e238e5e/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/16397a37e238e5e/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/16397a37e238e5e/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2015-01-02T18:45:48Z

Fixes #3120. Really nice work!

Snuffleupagus · 2015-01-02T22:40:53Z

web/viewer.js

@@ -1221,6 +1221,23 @@ var PDFViewerApplication = {
    }
  },

+  refreshSidebar: function pdfViewOpenSidebar() {


Nit: refreshSidebar seems like a slightly strange name, since the function only updates the thumbnails. How about refreshThumbnailViewer or something similar?

fkaelberer · 2015-01-03T09:32:39Z

@Snuffleupagus I agree. Commit is updated.

fkaelberer · 2015-01-04T16:25:03Z

I also changed the css style of the hidden sidebar container from visibility: hidden (= do the reflow and show things transparently) to display: none. This saves layouting of 4 extra DOM elements per page and so makes everything even smoother (it almost makes the first two commits obsolete ;-)

Snuffleupagus · 2015-01-04T16:49:56Z

I also changed the css style of the hidden sidebar container from visibility: hidden (= do the reflow and show things transparently) to display: none.

This is a great idea, but unfortunately it causes the "open sidebar" animation to behave strangely. The sidebar now opens instantaneously, while the toolbar still transitions correctly. Interestingly the "close" animation still works as it should.
To easily reproduce this, try increasing the transition-duration to a couple of seconds (instead of the current 200ms).

fkaelberer · 2015-01-05T08:06:57Z

@Snuffleupagus You're right, good point. It seems like this cannot be fixed in pure CSS, because the display property does not support transition. Since there's no trivial fix to this, I removed the commit again.

So I see three possibilities for this change:

Reject the idea
It might be possible to fix the transition with JavaScript and a timer (less elegant. I'd open a separate PR)
Somebody has a more elegant idea.

Reasons for 1: It's only relevant in edge cases, where documents have thousands of pages and thumbs are off. And performance issues are mostly resolved already in this PR. Solution 2 is kinda hacky.

Reasons for 2: A slimmer DOM tree is always welcome, and removing thousands of DOM elements from the layout are definitely noticeable in the UI responsiveness for large documents. Also saves some memory (1KB / page).

Any opinions?

CodingFabian · 2015-01-05T10:41:05Z

preferring display: none over visibility hidden is a good idea. it should be worked on in a follow up pr.

Snuffleupagus · 2015-01-05T10:59:17Z

My opinion is that we should not regress the current behaviour. I think we can try to implement the display: none (or similar) idea in a follow-up PR instead.

Snuffleupagus · 2015-01-05T12:26:38Z

@fkaelberer Will manually setting the height/width to 0 instead of using display: none achive the same result? If so, doing something like this might just work:

diff --git a/web/viewer.css b/web/viewer.css
index 91e64ea..affcc9a 100644
--- a/web/viewer.css
+++ b/web/viewer.css
@@ -166,7 +166,8 @@ html[dir='rtl'] .innerCenter {
   top: 0;
   bottom: 0;
   width: 200px;
-  visibility: hidden;
+  max-height: 0px;
+  max-width: 0px;
   -webkit-transition-duration: 200ms;
   -webkit-transition-timing-function: ease;
   transition-duration: 200ms;
@@ -186,7 +187,8 @@ html[dir='rtl'] #sidebarContainer {

 #outerContainer.sidebarMoving > #sidebarContainer,
 #outerContainer.sidebarOpen > #sidebarContainer {
-  visibility: visible;
+  max-height: inherit;
+  max-width: inherit;
 }
 html[dir='ltr'] #outerContainer.sidebarOpen > #sidebarContainer {
   left: 0px;

fkaelberer · 2015-01-06T08:28:47Z

Needs merge.

fkaelberer · 2015-01-06T22:13:03Z

Merged.

CodingFabian · 2015-01-11T12:55:54Z

I think together with #5617 this is good to go.

fkaelberer · 2015-01-11T18:08:06Z

I reported the scrolling performance regression of FF37 upstream:
https://bugzilla.mozilla.org/show_bug.cgi?id=1117322

timvandermeij · 2015-01-26T20:51:45Z

@fkaelberer Could you rebase this since the thumbnail code has been refactored in #5673?

fkaelberer · 2015-01-27T20:26:41Z

Rebased.

yurydelendik · 2015-02-27T12:50:03Z

/botio-windows preview

pdfjsbot · 2015-02-27T12:50:03Z

From: Bot.io (Windows)

Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/a93b27c12834f05/output.txt

pdfjsbot · 2015-02-27T12:50:57Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/a93b27c12834f05/output.txt

Total script time: 0.88 mins

Published

…enVisible Update thumbnails only when they are visible (to improve scrolling through large documents)

yurydelendik · 2015-02-27T12:56:37Z

Thank you

…sOnlyWhenVisible Update thumbnails only when they are visible (to improve scrolling through large documents)

timvandermeij added viewer performance labels Dec 27, 2014

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from d5dd27f to 9aa3b24 Compare December 27, 2014 16:40

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 9aa3b24 to 4271bf4 Compare December 28, 2014 18:25

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 4271bf4 to 6f99d68 Compare January 2, 2015 17:42

fkaelberer changed the title ~~Update thumbnail style only when they are visible (to improve scrolling through large documents)~~ Update thumbnails only when they are visible (to improve scrolling through large documents) Jan 2, 2015

Snuffleupagus reviewed Jan 2, 2015
View reviewed changes

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 6f99d68 to cd22b9b Compare January 3, 2015 09:31

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from cd22b9b to 30b3bb4 Compare January 4, 2015 16:19

fkaelberer mentioned this pull request Jan 4, 2015

Scrolling for PDF file is slow #3120

Closed

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 30b3bb4 to 4555b56 Compare January 5, 2015 07:40

fkaelberer mentioned this pull request Jan 5, 2015

Don't layout sidebar when hidden #5617

Closed

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 4555b56 to 6aa5d5c Compare January 6, 2015 22:10

fkaelberer added 2 commits January 27, 2015 21:08

Update thumbnail style only when sidebar is visible

047d61e

Update thumbnail images only when sidebar is visible

d3022e1

fkaelberer force-pushed the updateThumbnailClassesOnlyWhenVisible branch from 6aa5d5c to d3022e1 Compare January 27, 2015 20:26

yurydelendik added a commit that referenced this pull request Feb 27, 2015

Merge pull request #5586 from fkaelberer/updateThumbnailClassesOnlyWh…

ce12259

…enVisible Update thumbnails only when they are visible (to improve scrolling through large documents)

yurydelendik merged commit ce12259 into mozilla:master Feb 27, 2015

speedplane pushed a commit to speedplane/pdf.js that referenced this pull request Mar 4, 2015

Merge pull request mozilla#5586 from fkaelberer/updateThumbnailClasse…

40ee730

…sOnlyWhenVisible Update thumbnails only when they are visible (to improve scrolling through large documents)

fkaelberer deleted the updateThumbnailClassesOnlyWhenVisible branch March 15, 2015 15:45

timvandermeij mentioned this pull request Mar 15, 2015

Poor performance on a pdf document #4626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update thumbnails only when they are visible (to improve scrolling through large documents) #5586

Update thumbnails only when they are visible (to improve scrolling through large documents) #5586

fkaelberer commented Dec 26, 2014

CodingFabian commented Dec 26, 2014

yurydelendik commented Dec 26, 2014

fkaelberer commented Dec 26, 2014

fkaelberer commented Dec 27, 2014

CodingFabian commented Dec 27, 2014

Snuffleupagus commented Dec 27, 2014

fkaelberer commented Dec 28, 2014

Snuffleupagus commented Dec 29, 2014

pdfjsbot commented Dec 29, 2014

pdfjsbot commented Dec 29, 2014

Snuffleupagus commented Dec 29, 2014

fkaelberer commented Jan 2, 2015

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

timvandermeij commented Jan 2, 2015

Snuffleupagus Jan 2, 2015

fkaelberer commented Jan 3, 2015

fkaelberer commented Jan 4, 2015

Snuffleupagus commented Jan 4, 2015

fkaelberer commented Jan 5, 2015

CodingFabian commented Jan 5, 2015

Snuffleupagus commented Jan 5, 2015

Snuffleupagus commented Jan 5, 2015

fkaelberer commented Jan 6, 2015

fkaelberer commented Jan 6, 2015

CodingFabian commented Jan 11, 2015

fkaelberer commented Jan 11, 2015

timvandermeij commented Jan 26, 2015

fkaelberer commented Jan 27, 2015

yurydelendik commented Feb 27, 2015

pdfjsbot commented Feb 27, 2015

pdfjsbot commented Feb 27, 2015

yurydelendik commented Feb 27, 2015

Update thumbnails only when they are visible (to improve scrolling through large documents) #5586

Update thumbnails only when they are visible (to improve scrolling through large documents) #5586

Conversation

fkaelberer commented Dec 26, 2014

CodingFabian commented Dec 26, 2014

yurydelendik commented Dec 26, 2014

fkaelberer commented Dec 26, 2014

fkaelberer commented Dec 27, 2014

CodingFabian commented Dec 27, 2014

Snuffleupagus commented Dec 27, 2014

fkaelberer commented Dec 28, 2014

Snuffleupagus commented Dec 29, 2014

pdfjsbot commented Dec 29, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 29, 2014

From: Bot.io (Linux)

Success

Published

Snuffleupagus commented Dec 29, 2014

fkaelberer commented Jan 2, 2015

timvandermeij commented Jan 2, 2015

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 2, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jan 2, 2015

Snuffleupagus Jan 2, 2015

Choose a reason for hiding this comment

fkaelberer commented Jan 3, 2015

fkaelberer commented Jan 4, 2015

Snuffleupagus commented Jan 4, 2015

fkaelberer commented Jan 5, 2015

CodingFabian commented Jan 5, 2015

Snuffleupagus commented Jan 5, 2015

Snuffleupagus commented Jan 5, 2015

fkaelberer commented Jan 6, 2015

fkaelberer commented Jan 6, 2015

CodingFabian commented Jan 11, 2015

fkaelberer commented Jan 11, 2015

timvandermeij commented Jan 26, 2015

fkaelberer commented Jan 27, 2015

yurydelendik commented Feb 27, 2015

pdfjsbot commented Feb 27, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 27, 2015

From: Bot.io (Windows)

Success

Published

yurydelendik commented Feb 27, 2015